fix(instrumentation-pg): preserve SQL template text property #3259
Conversation
| // TODO: remove the `as ...` casts below when the TS version is upgraded. | ||
| // Newer TS versions will use the result of firstArgIsQueryObjectWithText | ||
| // to properly narrow arg0, but TS 4.3.5 does not. | ||
| const queryConfig = firstArgIsString | ||
| ? { | ||
| text: arg0 as string, | ||
| values: Array.isArray(args[1]) ? args[1] : undefined, | ||
| } | ||
| : firstArgIsQueryObjectWithText | ||
| ? { | ||
| ...(arg0 as any), | ||
| values: | ||
| (arg0 as any).values ?? | ||
| (Array.isArray(args[1]) ? args[1] : undefined), | ||
| ? (() => { | ||
| const query = arg0 as any; | ||
| // If the query object has no values yet, use the second argument as values | ||
| if (query.values === undefined && Array.isArray(args[1])) { | ||
| query.values = args[1]; | ||
| } | ||
| : undefined; | ||
| return query; | ||
| })() | ||
| : undefined; |
There was a problem hiding this comment.
I already did not like this ternary soup, so if you don't mind I'd like to use this opportunity to refactor it into something more readable rather than adding an IIFE into the mix 😅
There was a problem hiding this comment.
Absolutely, a clean refactor sounds like the right move 👍
There was a problem hiding this comment.
Another thing, there is another spread happening here. Could you add a test checking SQL template literals with added comments too ?
There was a problem hiding this comment.
Definitely, I’ll expand the test to cover SQL template literals with added comments too. Appreciate you catching the spread.
raphael-theriault-swi
left a comment
raphael-theriault-swi
left a comment
There was a problem hiding this comment.
Thanks for working on this !
|
Thank you for your contribution @aryamohanan! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
3 participants
Which problem is this PR solving?
closes #3258
Fixes crash in the pg instrumentation when using SQL template literal library (sql-template-strings).
TypeError: Cannot read properties of undefined (reading 'indexOf') at parseNormalizedOperationNameThe issue was introduced by #3196 , where spreading query objects caused SQL template objects to lose their
textgetter, leading toparseNormalizedOperationName()receivingundefinedand throwing the error.Short description of the changes
queryConfiginstead of spreading itvaluesfrom the second argument only if missingclient.query(SQL\...`)` workscorrectly and spans are recorded